Remove most old example code#470
Conversation
2c5dda5 to
08f2c6f
Compare
pelesh
left a comment
pelesh
left a comment
There was a problem hiding this comment.
Rather than removing examples, I would turn them into integration tests. Some of those examples set up GridKit apps without parser and thus verify that the system composer generates system models correctly and integrates well with solvers.
I added them in CaseFormatTests to compare parsing results with the direct assignment that was in those examples. I thought this was what we had discussed before, but maybe I misunderstood. Do you want something more than that? |
91f8a14 to
5b2e2fd
Compare
e8a9eba to
4b7d081
Compare
|
This would be nice to merge soon in preparation for the updated case data and documentation |
I suggest we move examples that do not use parser to the integration tests directory. They have educational value as they show what happens under the hood and how the system is assembled.
|
4b7d081 to
5c474f5
Compare
|
@nkoukpaizan, @pelesh this is ready to go |
5c474f5 to
13cc873
Compare
There was a problem hiding this comment.
Mostly looks good to me. A couple of minor commnents.
I also noticed the ThreeBusZipLoadJson is missing an add_test and updated reference file implementation. @PhilipFackler Just so this is complete for TwoBus and ThreeBus examples, would you mind converting that example as well?
| ], | ||
| "reference_file": "TwoBusBasic.ref.csv", | ||
| "error_tolerance": 2.03e-4 | ||
| "error_tolerance": [2.01e-4, 1e-4] |
There was a problem hiding this comment.
Good for compatibility with the current implementation. I would question the value keeping per-variable errors, especially since this does not scale to larger cases. cc @lukelowry @abirchfield
There was a problem hiding this comment.
It IS optional...and a little bit secret. :)
There was a problem hiding this comment.
As long as it's optional, it certainly does not hurt to have. Bless you sir
Co-authored-by: Nicholson Koukpaizan <72402802+nkoukpaizan@users.noreply.github.com>
ac90307 to
86fa582
Compare
nkoukpaizan
left a comment
nkoukpaizan
left a comment
There was a problem hiding this comment.
Looks good to me! I'll give others some time to review before merging.
lukelowry
left a comment
lukelowry
left a comment
There was a problem hiding this comment.
Much needed cleanup thank you for doing this, quite appreciated.
4 participants
Description
Remove example code that duplicates what the
DynamicSimulationapp doesCloses #390 (indirectly)
Proposed changes
CaseFormatTeststo check JSON parserref.csvfiles for cases that were still using the.hppdata.solver.jsonfiles where neededChecklist
-Wall -Wpedantic -Wconversion -Wextra.